Add group to frontend and dcar match summary page data models#15079
Add group to frontend and dcar match summary page data models#15079marjisound merged 4 commits intomainfrom
Conversation
marjisound
changed the title
marjisound
force-pushed
the
add-group-to-football-match-page-data-model
branch
from
b989524 to
ac9c16c
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
JamieB-gu
left a comment
JamieB-gu
left a comment
There was a problem hiding this comment.
Looks good, one question.
|
|
||
| export type FEFootballMatchPage = FEFootballDataPage & { | ||
| footballMatch: FEFootballMatch; | ||
| group?: FEGroupSummary; |
There was a problem hiding this comment.
Are there occasions when we don't have a group for a particular match?
There was a problem hiding this comment.
Good question, I think we expect to have league table for all our competitions, but in our data, we are considering the possibility of the league table to be empty and use hasLeagueTable to indicate if the competition has a league table.
Now I'm thinking what should we do in a case like the match summary (match info) page? Do you think it might make more sense to make the group a mandatory field?
For now I merge this, but lets talk about again. I created this issue so we won't forget this #15204
github-actions
bot
removed
the
run_chromatic
|
Seen on PROD (merged by @marjisound 10 minutes and 7 seconds ago) Please check your changes! |
2 participants
What does this change?
This PR
FEFootballMatchPagewith a new fieldgroupFEGroupSummary&FELeagueTableEntrySummaryto remove results from entryFootballTableSummary&EntrySummaryThis is because for the new design of the match info (summary) page, we need to include a summary of the league table group rather than the whole league table. Also to optimise performance and payload size, the new *Summary types explicitly exclude detailed match results, as they are not required for this view.
The frontend PR for this guardian/frontend#28495
Test
This was tested locally and in code. Currently it doesn't have any impact on the page since the new component
FootballMatchInfois not yet wired up within the pagesThis PR fixes part of #14904
IMPORTANT NOTE
I found a bug in the change I made in frontend guardian/frontend#28495 to update the data model. I fixed it in a second PR guardian/frontend#28542. So this DCAR PR can ONLY be merged after guardian/frontend#28542